-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Move listeners module to shared library for client server separation #59883
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
93d3cf8 to
11403fb
Compare
1d8a0b8 to
c463664
Compare
potiuk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good - I tnink dagnode leaked from another branch - but other than that - it's exactly how I imagined it :)
|
Also - there are two remaining questions about tests -> I am not sure why we have task.sdk in airflow-core unit tests ? |
|
@kaxil there's a request changes from you over here, could you take a look again? Or remove the request change if your veto was handled? |
uranusjr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaning up changeset
8a02228 to
4a63f94
Compare
…pache#59883) Extract the listeners infrastructure to `shared/listeners/` library to eliminate cross dependencies between airflow-core and task-sdk. - ListenerManager and hookimpl marker now in shared library - Hook specs split by callers: - shared: lifecycle, taskinstance (called from both sdk and core) - core: dagrun, asset, importerrors (called only from core) - sdk registers only specs it actually uses (lifecycle, taskinstance) - core registers all specs for full listener support
…pache#59883) Extract the listeners infrastructure to `shared/listeners/` library to eliminate cross dependencies between airflow-core and task-sdk. - ListenerManager and hookimpl marker now in shared library - Hook specs split by callers: - shared: lifecycle, taskinstance (called from both sdk and core) - core: dagrun, asset, importerrors (called only from core) - sdk registers only specs it actually uses (lifecycle, taskinstance) - core registers all specs for full listener support
…pache#59883) Extract the listeners infrastructure to `shared/listeners/` library to eliminate cross dependencies between airflow-core and task-sdk. - ListenerManager and hookimpl marker now in shared library - Hook specs split by callers: - shared: lifecycle, taskinstance (called from both sdk and core) - core: dagrun, asset, importerrors (called only from core) - sdk registers only specs it actually uses (lifecycle, taskinstance) - core registers all specs for full listener support
closes: #54701
Why?
Simply stating, for client server separation.
The
airflow.listenersmodule was used between airflow-core and task-sdk. By extracting the shared components to a shared library, I am intending to solve the problem of cross dependency between airflow-core and task-sdk.What's done?
Created
shared/listeners/library containing:ListenerManagerclass (manages plugin registration and hook dispatching)hookimplmarker for implementing listenersHook specs are now houses by where they're called from:
Both airflow-core and task-sdk now:
_shared/listeners/)SDK now only registers specs it actually uses:
lifecycle(on_starting, before_stopping)taskinstance(on_task_instance_*)Open Questions and Direction to proceed
Type hints in hook specs
The specs use TYPE_CHECKING imports to avoid runtime dependencies:
taskinstancespec uses union typeRuntimeTaskInstance | TaskInstancesince it's called from both SDK workers and Core API serverTODO
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.